Try to bring up the most recently used NetworkManager connection profiles #101
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @coiby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily addresses a critical issue in the nfs_ovs kdump plan where NetworkManager fails to activate the correct network connection (bonding) during initrd. It resolves this by ensuring NetworkManager can properly prioritize connections based on recent usage. Additionally, the PR expands the project's test coverage to include aarch64 architecture and rawhide distributions, and refactors existing test plans for better organization.
Highlights
- NetworkManager Connection Prioritization Fix: Resolved an issue where NetworkManager failed to activate the correct bonding network connection in the
nfs_ovskdump plan. This was achieved by including the/var/lib/NetworkManager/timestampsfile in the initrd, allowing NetworkManager to correctly prioritize connection profiles based on their most recent usage. - Expanded Test Coverage: Extended automated build and test targets in
.packit.yamlto includeaarch64architecture for both Fedora 41 and Rawhide, significantly broadening the platform coverage for testing. - Test Plan Refactoring: Reorganized test plans by introducing a new
early.fmfplan for kdump early dumping tests and removing the deprecatednfs_early.fmfplan, streamlining the test suite structure. - Integration Test Update: Modified the
tools/run-integration-tests.shscript to explicitly execute thelvm2_thinptest plan, ensuring specific test scenarios are covered.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to resolve a NetworkManager connection activation priority issue by including /var/lib/NetworkManager/timestamps in the kdump initrd. The changes also update CI and test configurations. My review focuses on the robustness of this change. I've identified a potential scenario where the script could fail if the timestamps file is missing and have suggested a fix to prevent this.
coiby
force-pushed
the
test_fix
branch
2 times, most recently
from
2481ed4 to
ac5fd72
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with NetworkManager connection activation priority in the kdump environment by including the /var/lib/NetworkManager/timestamps file in the initrd. This allows for the correct prioritization of recently used network connections. The changes also include updates to the CI test matrix to cover more targets and some test plan refactoring. The core change is sound, but I have one suggestion to improve its robustness.
…iles Resolves: https://issues.redhat.com/browse/KEX-344 Currently, the nfs_ovs plan fails because nm-wait-online-initrd.service waits for the bonding network to be created. But the bonding network can't be activated because the eth0 NM connection is brought up first which stops the slave connnection profile to be activated, /var/lib/NetworkManager/timestamps keeps the timestamps of NM connections when they are lastly used. When multiple NM connections have the same connection.autoconnect-priority, the latest used NM connection will be activated. [1] https://networkmanager.dev/docs/api/latest/settings-connection.html Fixes: 3f4a9eb ("Bring up the network before Open vSwitch bridge") Signed-off-by: Coiby Xu <[email protected]>
The plan is to run kernel-tests in upstream kdump-utils. Use config-earlykdump from kernel-tests so we can retire current nfs_early plan. Signed-off-by: Coiby Xu <[email protected]>
I plan to use packit to run all test plans. But we still need to run lvm2_thinp test plan on self-hosted runner before we adapt it to packit/testing-farm. Also enable tests against Fedora-41 since tmt no longer supports Fedora-40. Signed-off-by: Coiby Xu <[email protected]>
Let's extend packit tests to rawhide and aarch64 to catch regressions early. Signed-off-by: Coiby Xu <[email protected]>
Occasionally testing farm will run the tests on a kind of HVM domU machines which doesn't support kdump. AWS EC2 T2 is known to not support kdump [1]. Exclude using Xen guests to avoid this issue. [1] https://issues.redhat.com/browse/RHEL-108739 Signed-off-by: Coiby Xu <[email protected]>
1 participant
Try to bring up the most recently used NetworkManager connection profiles
Resolves: https://issues.redhat.com/browse/KEX-344
Currently, the nfs_ovs plan fails because nm-wait-online-initrd.service
waits for the bonding network to be created. But the bonding network
can't be activated because the eth0 NM connection is brought up first
which stops the slave connnection profile to be activated,
/var/lib/NetworkManager/timestamps keeps the timestamps of NM
connections when they are lastly used. When multiple NM connections have
the same connection.autoconnect-priority, the latest used NM connection
will be activated.
Note this PR also enable tests for rawhide and aarch64.
[1] https://networkmanager.dev/docs/api/latest/settings-connection.html
Fixes: 3f4a9eb ("Bring up the network before Open vSwitch bridge")